Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

VersionMap must contain all structs at their current versions #5

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

VersionMap must contain all structs at their current versions #5

wants to merge 1 commit into from

Conversation

ioanachirca
Copy link
Contributor

@ioanachirca ioanachirca commented Jun 18, 2020 •
edited
Loading

Signed-off-by: Ioana Chirca [email protected]

If we are (de)serializing at the latest version from VersionMap, check that the struct's current version matches the version from the map. If it doesn't, it means that the Version Map is outdated.

Running cargo bench in firecracker/src/snapshot crate after updating the versionize_derive dependency gives us these mean times:

Test Mean
Serialize 320.47 us
Serialize + crc64 378.65 us
Deserialize 59.05 us
Deserialize + crc64 224.21 us

The extra checks do not introduce a regression (current baseline here)

Description of Changes

  • add a check for outdated Version Map
  • bump the crate version (the check requires VersionizeError to be in scope, which is not necessarily true for the crates using versionize_derive)

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.

PR Checklist

[Author TODO: Meet these criteria.]
[Reviewer TODO: Verify that these criteria are met. Request changes if not]

  • All commits in this PR are signed (git commit -s).
  • The reason for this PR is clearly provided (issue no. or explanation).
  • The description of changes is clear and encompassing.
  • Any required documentation changes (code and docs) are included in this PR.
  • Any newly added unsafe code is properly documented.
  • Any user-facing changes are mentioned in CHANGELOG.md.

@ioanachirca ioanachirca mentioned this pull request Jun 18, 2020
Open
6 tasks
@sandreim
Copy link
Contributor

Changelog update required.

@ioanachirca ioanachirca force-pushed the enforce_latest branch 3 times, most recently from 767d0b3 to 9c4fa73 Compare June 25, 2020 11:24
sandreim
sandreim previously approved these changes Jun 25, 2020
View reviewed changes
lauralt
lauralt reviewed Jun 26, 2020
View reviewed changes
Copy link

@lauralt lauralt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Some comments should be modified and also the commit message should be updated with the struct/union/enum observation (plus verion typo).

src/helpers.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@ioanachirca
Ensure latest version from VersionMap contains...
003a83d 
...all structs/enums/unions at their current versions.

This commit also bumps the crate version to 0.1.2

Signed-off-by: Ioana Chirca <[email protected]>
lauralt
lauralt reviewed Jun 26, 2020
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
alexandruag
alexandruag reviewed Jun 26, 2020
View reviewed changes
Copy link

@alexandruag alexandruag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good, but has a bit of downside in the sense it doesn't help with structures where we implement Versionize manually. Looks like doing the check in VersionMap::get_type_version itself covers more cases. That's not really possible right now because that method expects the type_id directly as a parameter, so we don't have access to the current version of the corresponding type. Would it make sense to change its signature to something like pub fn get_type_version<T: Versionize>(&self, root_version: u16) -> u16?

@sandreim
Copy link
Contributor

This looks good, but has a bit of downside in the sense it doesn't help with structures where we implement Versionize manually. Looks like doing the check in VersionMap::get_type_version itself covers more cases. That's not really possible right now because that method expects the type_id directly as a parameter, so we don't have access to the current version of the corresponding type. Would it make sense to change its signature to something like pub fn get_type_version<T: Versionize>(&self, root_version: u16) -> u16?

This also mitigates the binary size increase due to the current solution with generated code.

@ioanachirca
Copy link
Contributor Author

This looks good, but has a bit of downside in the sense it doesn't help with structures where we implement Versionize manually. Looks like doing the check in VersionMap::get_type_version itself covers more cases. That's not really possible right now because that method expects the type_id directly as a parameter, so we don't have access to the current version of the corresponding type. Would it make sense to change its signature to something like pub fn get_type_version<T: Versionize>(&self, root_version: u16) -> u16?

Sounds better this way, thanks for pointing this out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexandruag alexandruag alexandruag left review comments

@lauralt lauralt lauralt left review comments

@sandreim sandreim sandreim left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ioanachirca @sandreim @alexandruag @lauralt